Adding SCSS Support #5
Conversation
mixin.scss
Outdated
|
|
||
| @mixin calculate-line-height-as-scale($line-height) { | ||
| $line-height-scale: ($line-height / ($bk-type-size-modifier * $sk-base-font-size)); | ||
| line-height: unit($line-height-scale, em); |
There was a problem hiding this comment.
unit($line-height-scale, em) does not seem compatible with SCSS, got it to work using ($line-height-scale * 1em) but not sure if that is what people do in the SCSS world. Was this working for you?
mixin.scss
Outdated
|
|
||
| @if type-of($bk-line-height-override) == number { | ||
| @include calculate-line-height-as-scale($bk-line-height-override); | ||
| @include calculate-type-offset($line-height-scale); |
There was a problem hiding this comment.
$line-height-scale is a variable created in the scope of the calculate-type-offset mixin, LESS let's you access it outside of the block scope but SCSS does not. You might have more luck modelling this file on the JS version as the LESS makes code like this a disaster!
|
Thanks for getting this underway. I just tried to run this through an scss compiler online (as i dont use it on any projects of my own) and it hit a few errors (see comments inline). |
|
Also worth noting I just released v3.0.0 which has some changes and I think simplifies some of the code, so might be worth revisiting modelling your code more on the JS version |
|
Sweet! I submitted this as a baseline to get started. Haven't fully vetted. I'll look through the 3.0 changes and get this up to date and working. |
|
Sounds great, really appreciate your initiative. As I mentioned running it through an online compiler my be an easy workflow to iterate on this if not directly consuming it in an app. I'm looking to do testing too which may aid tasks like this in the future. |
|
@michaeltaranto - refactored sass version to work a bit more like the javascript version. Found it easiest to create the function as a mixin which uses 2 additional functions (one to calculate the type offset and one to render the final values). DRY in SASS FTW. Worth noting, you have to pass the mixin with parameters numbers without units (no px or em). If you are feeding .my__heading{
@include basekick(
1.4,
3,
.14,
10,
9
)
}TestingHad some trouble getting your docs working locally to a point it was easy to test, so I set up a small project myself with a SCSS and JS file using Webpack. Happy to share later! SASS described above In JS: import basekick from '../helpers/basekick.js';
let options = {
typeSizeModifier: 1.4,
typeRowSpan: 3,
descenderHeightScale: .14,
baseFontSize: 10,
gridRowHeight: 9,
lineHeightOverride: false
}
console.log(basekick(options));Resultsconsole.log {fontSize: "14px", lineHeight: "27px", transform: "translateY(0.6042857142857143em)"}rendered css: .my__heading {
font-size: 14px;
line-height: 27px;
-ms-transform: translateY(0.60429em);
transform: translateY(0.60429em);
} |
|
Hey @michaeltaranto - had a chance to review this yet? |
| @@ -0,0 +1,32 @@ | |||
| @function bk-calculateTypeOffset($line-height, $font-size, $descdender-height-scale){ | |||
There was a problem hiding this comment.
You have a typo here, should be $descender-height-scale
| @@ -0,0 +1,32 @@ | |||
| @function bk-calculateTypeOffset($line-height, $font-size, $descdender-height-scale){ | |||
| $line-height-scale: $line-height / $font-size; | |||
| $line-height-scale: (($line-height-scale - 1) / 2) + $descdender-height-scale; | |||
There was a problem hiding this comment.
Rather than mutate the $line-height-scale variable here, you can just return this line right?
| font-size: $font-size * 1px; | ||
| line-height: $line-height * 1px; | ||
| transform: translateY( $type-offset * 1em ); | ||
| } |
There was a problem hiding this comment.
My SASS experience is non-existent so excuse my ignorance, but any reason you dont place these functions inside the basekick mixin? Are they global when they sit out here?
| $line-height: $bk-grid-row-height * $bk-type-row-span; | ||
| $type-offset: bk-calculateTypeOffset($line-height, $font-size, $bk-descender-height-scale); | ||
| @include bk-render($font-size, $line-height, $type-offset); | ||
| } |
There was a problem hiding this comment.
I would propose cleaning up some of this duplication by using a more ternary like syntax for the line-height/line-height-override:
$line-height: if(type-of($bk-line-height-override) == number,
$bk-line-height-override,
$bk-grid-row-height * $bk-type-row-span);Then you would not need to duplicate the $type-offset calculation, and could arguably do away with the bk-render mixin and just have the properties inline.
|
Sorry it took me a while to get to this, had the flu and been playing catch up. Much prefer this approach, i think a few tidy ups and we are good to go. Yeah the demo quite hardcoded (actually out of date too). I want to create a mini site on github pages at some point when time allows. That would give a nice test bed for changes too. |
|
@carsonjones @michaeltaranto |
|
Go for it @kimneu ! |
Opening pull to start SCSS support.